Tests: Usermod rename user, change password, lock password#1622
Tests: Usermod rename user, change password, lock password#1622aborah-sudo wants to merge 4 commits into
Conversation
aborah-sudo
changed the title
aborah-sudo
force-pushed
the
usermode
branch
7 times, most recently
from
1df807b to
83bcf72
Compare
aborah-sudo
force-pushed
the
usermode
branch
6 times, most recently
from
d06e12d to
0611ed0
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
I see you've chosen passlib over my suggestion to use hashlib. Can you help me understand the technical reasoning behind this choice? I want to make sure I understand the requirements correctly.
In addition, several commit messages mention usermode, but that's a different binary that isn't provided by shadow. Do you mind updating the commit messages to usermod?
aborah-sudo
force-pushed
the
usermode
branch
4 times, most recently
from
7f9d5a6 to
e761230
Compare
|
I see we went back to the I also want to avoid Please implement this using |
aborah-sudo
force-pushed
the
usermode
branch
3 times, most recently
from
6d48bb4 to
7b7a520
Compare
Done |
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
There's a critical issue with the current password hash generation approach that needs to be addressed. While the code that you implemented produces a string that looks like a SHA-512 crypt hash (starting with $6$), it's not actually a valid SHA-512 crypt hash according to the crypt(3) specification. The issue is that hashlib.sha512() performs a simple single-pass hash, whereas the SHA-512 crypt algorithm requires thousands of rounds of key stretching (typically 5000+ iterations) and uses a specific base64 encoding with a custom alphabet. Real SHA-512 crypt hashes have exactly 86 base64-encoded characters after the salt, not 128 hex characters.
The practical impact is that while usermod -p will accept and store your generated hash (since it doesn't validate the format), users would never be able to authenticate with passwords set this way because the system's authentication libraries expect real crypt-format hashes.
That means i have to use passlib as i used before ? passlib is the officially recommended replacement for the crypt module. It provides cross-platform support for SHA-512 crypt hashes. |
Added passlib dependency for generating valid SHA-512 crypt hashes
This is the transformation to Python of the test located in `tests/usertools/01/10_usermod_rename_user_in_group.test` which checks that `usermod` can rename user who is member of a group
This is the transformation to Python of the test located in `tests/usertools/01/11_usermod_change_password.test` which checks that `usermod` can change user password
This is the transformation to Python of the test located in `tests/usertools/01/11_usermod_lock_password.test` which checks that `usermod` can lock user password
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
I added some comments inline.
For future PRs, please provide more technical reasoning when choosing (or switching) between libraries. We went through several iterations here because the 'why' behind the implementation wasn't clear. To keep the review process efficient, it’s helpful if you argue your decisions or explain your logic upfront rather than just making silent changes.
| shadow_entry = shadow.tools.getent.shadow("tuser1") | ||
| assert shadow_entry is not None, "User should be found" | ||
| assert shadow_entry.password is not None, "Password should not be None" | ||
| assert shadow_entry.password.startswith("$6$"), "Password should be SHA-512 crypt hash" |
There was a problem hiding this comment.
In addition to these asserts you should also assert the password validity. You can do this by checking the validity itself with shadow_password_pattern() or by comparing with the hash that you already generated shadow_entry.password == password_hash
|
|
||
| shadow_entry = shadow.tools.getent.shadow("tuser1") | ||
| assert shadow_entry is not None, "User should be found" | ||
| assert shadow_entry.password == password_hash, "Password should be unlocked" |
There was a problem hiding this comment.
Also check assert shadow_entry.password is not None, "Password should not be None" before checking the password itself
2 participants
No description provided.